Code refactor #408
Code refactor #408
Conversation
| @@ -81,7 +81,7 @@ bool anyPmem(const ServerUncoreMemoryMetrics & metrics) | |||
|
|
|||
| bool skipInactiveChannels = true; | |||
|
|
|||
| void print_help(const string prog_name) | |||
| void print_help(const string & progname) | |||
| { | |||
| cerr << "\n Usage: \n " << progname | |||
There was a problem hiding this comment.
Can you merge this change into the previous change? I would like each change to compile on its own and it is clear that this cannot compile on its own and needs both changes
There was a problem hiding this comment.
the change of the parameter name is not necessary and requires changing the name in other code lines. Could you please keep the old name?
| @@ -79,7 +79,7 @@ double getAverageUncoreFrequencyGhz(const UncoreStateType& before, const UncoreS | |||
| return getAverageUncoreFrequency(before, after) / 1e9; | |||
| } | |||
|
|
|||
| void print_help(const string prog_name) | |||
| void print_help(const string & progname) | |||
| { | |||
| cerr << "\n Usage: \n " << progname | |||
There was a problem hiding this comment.
Can you merge this change into the previous change? I would like each change to compile on its own and it is clear that this cannot compile on its own and needs both changes
There was a problem hiding this comment.
same here. Please keep the old name.
| @@ -977,7 +977,6 @@ class PCM_API PCM | |||
| auto ctrl = pmu.counterControl[c]; | |||
| if (ctrl.get() != nullptr) | |||
| { | |||
| *ctrl = MC_CH_PCI_PMON_CTL_EN; | |||
There was a problem hiding this comment.
I would prefer if you keep " *ctrl = MC_CH_PCI_PMON_CTL_EN;" because IIRC some PMU docs suggested that the PMU control register needs to be written without the event/umask and then in the next write the event/umask needs to be written.
| @@ -81,7 +81,7 @@ bool anyPmem(const ServerUncoreMemoryMetrics & metrics) | |||
|
|
|||
| bool skipInactiveChannels = true; | |||
|
|
|||
| void print_help(const string prog_name) | |||
| void print_help(const string & progname) | |||
| { | |||
| cerr << "\n Usage: \n " << progname | |||
There was a problem hiding this comment.
the change of the parameter name is not necessary and requires changing the name in other code lines. Could you please keep the old name?
| @@ -79,7 +79,7 @@ double getAverageUncoreFrequencyGhz(const UncoreStateType& before, const UncoreS | |||
| return getAverageUncoreFrequency(before, after) / 1e9; | |||
| } | |||
|
|
|||
| void print_help(const string prog_name) | |||
| void print_help(const string & progname) | |||
| { | |||
| cerr << "\n Usage: \n " << progname | |||
There was a problem hiding this comment.
same here. Please keep the old name.
| } | ||
| if (m->localMemoryRequestRatioMetricAvailable()) { | ||
| cout << " LOCAL : ratio of local memory requests to memory controller in %\n"; | ||
| cout << "LLCRDMISSLAT: average latency of last level cache miss for reads and prefetches (in ns)\n"; |
There was a problem hiding this comment.
the availability of LLCRDMISSLAT metric should be checked with m->LLCReadMissLatencyMetricsAvailable() and not with m->localMemoryRequestRatioMetricAvailable()
|
@GermanAizek ping |
@rdementi,
Check my changes, thanks.
Feedback with me with comments.